-
Notifications
You must be signed in to change notification settings - Fork 2.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[ir][refactor] First step to move Frontend IR to its own file #914
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks! And yes, let's move the Expressions as well. The expressions are only useful in the frontend before lower_ast
. Not sure if we should create a separate file for the Expressions, but you should clearly have better judgment than I do! :-)
Cool! Not sure either, but I don't see an immediate need for separate Frontend Stmt and Expressions, so I will go with one file... |
Sounds good! Given the current code structure, I don't think separating Exprs and Stmts is an easy job anyway... |
Will create another PR for the rest of the works.. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
@@ -7,6 +7,7 @@ | |||
#include <unordered_map> | |||
|
|||
#include "taichi/ir/frontend.h" | |||
#include "taichi/ir/frontend_ir.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if this is necessary after we move all Frontend*Stmt
s to their own file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess they have to be around for a while, since currently there are some legacy stuff referring to the Frontend stmts, e.g.
Lines 933 to 941 in 4d228b9
For::For(const ExprGroup &i, | |
const Expr &global, | |
const std::function<void()> &func) { | |
auto stmt_unique = std::make_unique<FrontendForStmt>(i, global); | |
auto stmt = stmt_unique.get(); | |
current_ast_builder().insert(std::move(stmt_unique)); | |
auto _ = current_ast_builder().create_scope(stmt->body); | |
func(); | |
} |
I think once these legacy constructs are removed, then it should be fine to remove this dependency..
In this PR, I just moved
FrontendAllocaStmt
to a new filefrontend_ir.h
. If this looks OK, I will move all the otherFrontend*Stmt
(maybe*Expression
as well ?)Related issue = #689
[Click here for the format server]